Skip to content

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 19, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

PR #14946 probably after running format.sh. The message with the next lines added is not consistent anymore, which causes the unit test to fail.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix


Description

  • Fixed the NumberFormatException message for Arabic locale.

  • Improved formatting of the exception message for clarity.

  • Ensured proper handling of port formatting in error messages.


Changes walkthrough 📝

Relevant files
Bug fix
DriverService.java
Refined `NumberFormatException` message for Arabic locale

java/src/org/openqa/selenium/remote/service/DriverService.java

  • Updated the NumberFormatException message for Arabic locale.
  • Refined the message to include formatted port numbers.
  • Enhanced clarity and readability of the exception message.
  • +4/-4     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Format String

    The String.format() call has a format specifier in the message but getPort() is passed as an argument that isn't used since the port is already formatted inline with String.format("--port=%d", port)

    throw new NumberFormatException(
        String.format(
            "Couldn't format the port numbers because the System Language is arabic: \""
                + String.format("--port=%d", port)
                + "\", please make sure to add the required arguments \"-Duser.language=en"
                + " -Duser.region=US\" to your JVM, for more info please visit :\n"
                + "  https://www.selenium.dev/documentation/webdriver/browsers/",
            getPort()));

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect string formatting usage

    The String.format() is used incorrectly. The first argument contains a placeholder
    (%d) but it's concatenated instead of being part of the format string. This will
    cause the getPort() parameter to be ignored.

    java/src/org/openqa/selenium/remote/service/DriverService.java [510-516]

     String.format(
    -    "Couldn't format the port numbers because the System Language is arabic: \""
    -        + String.format("--port=%d", port)
    -        + "\", please make sure to add the required arguments \"-Duser.language=en"
    -        + " -Duser.region=US\" to your JVM, for more info please visit :\n"
    -        + "  https://www.selenium.dev/documentation/webdriver/browsers/",
    -    getPort());
    +    "Couldn't format the port numbers because the System Language is arabic: \"--port=%d\", please make sure to add the required arguments \"-Duser.language=en -Duser.region=US\" to your JVM, for more info please visit :\n  https://www.selenium.dev/documentation/webdriver/browsers/",
    +    port);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a critical bug where nested String.format() usage would cause the port number to be ignored in the final formatted string, potentially leading to incorrect error messages. The proposed fix properly integrates the port parameter into a single format string.

    9

    @VietND96 VietND96 merged commit 4461033 into trunk Jan 19, 2025
    11 of 12 checks passed
    @VietND96 VietND96 deleted the exception-message branch January 19, 2025 23:12
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants